Skip to content

Fix incorrectly using 0-indexed number where 1-indexed number is expected#3397

Open
u9g wants to merge 6 commits intographql:mainfrom
u9g:u9g-patch-1
Open

Fix incorrectly using 0-indexed number where 1-indexed number is expected#3397
u9g wants to merge 6 commits intographql:mainfrom
u9g:u9g-patch-1

Conversation

@u9g
Copy link
Copy Markdown

@u9g u9g commented Aug 8, 2023

This pr causes console spam in our case for when we hover a comment on the first line. See obi1kenobi/trustfall#285 for more context.

What does this pr do?

Fixes an off-by-one bug that caused console spam when hovering things in the first line of a graphql file. This occurs because monaco's toGraphQLPosition() from monaco-graphql returns 0-based line/col numbers and getRange() from graphql-language-service expects 1-based line/col numbers.

Why is this the right fix?
We have a 0-indexed row/col. To see this for myself, I put a console.log() after this line. I then hovered the first character on the first line. I see 0,0 printed, and this is after we subtract 1 from both line/col in toGraphQLPosition(), so we definitely have a 0-indexed number.

getRange() takes in a SourceLocation, however that's an interface, so it's not really obvious whether the line/col is 0-based or 1-based. However, there is a function getLocation() that returns a SourceLocation declared in the same location.d.ts from within the graphql/language repo. So taking a look at their getLocation() function, it looks like this:

function getLocation(source, position) {
  let lastLineStart = 0;
  let line = 1;

  for (const match of source.body.matchAll(LineRegExp)) {
    typeof match.index === 'number' || (0, _invariant.invariant)(false);

    if (match.index >= position) {
      break;
    }

    lastLineStart = match.index + match[0].length;
    line += 1;
  }

  return {
    line,
    column: position + 1 - lastLineStart,
  };
}

When I saw that, it became clear that this line is 1-indexed, and the + 1 in the column leads me to believe this number is 1-based too. (if this isn't, please tell me!)

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Aug 8, 2023

🦋 Changeset detected

Latest commit: 1d988c1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
monaco-graphql Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented Aug 8, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@acao
Copy link
Copy Markdown
Member

acao commented Aug 8, 2023

@u9g thanks, this also fixes a bug in monaco-graphql iirc, can you add a changeset?

@u9g
Copy link
Copy Markdown
Author

u9g commented Aug 8, 2023

@u9g thanks, this also fixes a bug in monaco-graphql iirc, can you add a changeset?

I added a changeset for Fix incorrect loop condition, should I add a changeset for fix error range handling bug too if it only fixed bugs in the test as far as I know?

@acao
Copy link
Copy Markdown
Member

acao commented Aug 9, 2023

huh. seeing the change you made to the language service makes me want to check for regressions in monaco-graphql and other places where we don't have tests yet

@acao
Copy link
Copy Markdown
Member

acao commented Aug 9, 2023

from what i can tell, this is a breaking change, so we need to give the graphql-language-service a major bump

@acao
Copy link
Copy Markdown
Member

acao commented Aug 9, 2023

yeah, looks from the e2e tests that youll need to handle the breaking change in codemirror-graphql as well

Comment thread .changeset/little-hats-explain.md Outdated
@u9g
Copy link
Copy Markdown
Author

u9g commented Aug 10, 2023

yeah, looks from the e2e tests that youll need to handle the breaking change in codemirror-graphql as well

I actually took another look at my though process and remade this pr, take a look at the pr description for why I made the changes.

@u9g u9g changed the title Fix incorrect loop condition Fix incorrectly using 0-indexed number where 1-indexed number is expected Aug 10, 2023
{
column: graphQLPosition.character,
line: graphQLPosition.line,
// we add one here because `getRange()` expects a 1-indexed position
Copy link
Copy Markdown
Member

@acao acao Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a nit but this would be very useful to have in this file as toMonacoPosition() in ./utils ? I think this is the only place where we need to invoke it, but that could change. Aside from hover, the rest of the language services we currently implement and will implement tend to use ranges.

Copy link
Copy Markdown
Contributor

@TallTed TallTed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to improve human clarity

Comment thread .changeset/little-hats-explain.md Outdated
acao and others added 6 commits January 7, 2024 17:18
* happy svelte parsing
* test range
* refactor tag parsing for readability & perf tweaks
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants